-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
API: dont infer freq in DTA/TDA arithmetic ops #33487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this an a change here?
@@ -865,7 +865,7 @@ def test_dt64arr_add_sub_td64ndarray(self, tz_naive_fixture, box_with_array): | |||
tdi = pd.TimedeltaIndex(["-1 Day", "-1 Day", "-1 Day"]) | |||
tdarr = tdi.values | |||
|
|||
expected = pd.date_range("2015-12-31", periods=3, tz=tz) | |||
expected = pd.date_range("2015-12-31", periods=3, tz=tz)._with_freq(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you not just pass freq='None'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated+green
for result in [idx - delta, np.subtract(idx, delta)]: | ||
assert isinstance(result, DatetimeIndex) | ||
tm.assert_index_equal(result, exp) | ||
assert result.freq == "2D" | ||
|
||
# When adding/subtracting an ndarray (which has no .freq), the result | ||
# does not infer freq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think u need to pass freq=None when constructing th index rather than using a private method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, first we need to fix the DTI constructor so that if you explicitly pass freq=None it wont override the arg's freq, ATM we get:
>>> dti = pd.date_range("2016-01-01", periods=3)
>>> pd.DatetimeIndex(dti, freq=None).freq
<Day>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i guess freq should be a no_default arg s can handle None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah. ill make a PR for that in the AM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working on the freq=no_default
branch and its hairier than anticipated. if we can push this through in the interim that'll help me keep momentum on the "check freq in assert_index_equal" project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sure, yeah like to avoid using _with_freq
in tests at all
freq inference is pure overhead for the Series/DataFrame op because freq gets discarded.